Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: ansible kinesis stream paginated shards bug #93

Merged
merged 3 commits into from
Dec 1, 2020
Merged

fix: ansible kinesis stream paginated shards bug #93

merged 3 commits into from
Dec 1, 2020

Conversation

sidpatel13
Copy link
Contributor

SUMMARY

"Fixes #92"

It looks like has_more_shards isn't really used other than for the purposes of this while loop so it is safe to set ExclusiveStartShardId to the last shard and pass those params at the next call of the while loop which subsequently sets has_more_shards to False. Otherwise, due to a kinesis stream having > 100 shards, has_more_shards will always be True making us stuck in this while loop and we'll get timeouts.

ISSUE TYPE
  • Bugfix Pull Request

@jillr jillr changed the base branch from master to main July 2, 2020 19:48
@ansibullbot
Copy link

cc @jillr @linuxdynasty @s-hertel @tremble @wimnat
click here for bot help

@ansibullbot ansibullbot added affects_2.10 bug This issue/PR relates to a bug community_review module module needs_triage new_contributor Help guide this first time contributor small_patch Hopefully easy to review stale_ci CI is older than 7 days, rerun before merging labels Aug 19, 2020
tremble
tremble requested changes Aug 20, 2020
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sidpatel13

Thanks for taking the time to submit the issue and this patch.

I think this change might work, we currently have a pending PR to add some integration tests for kinesis_stream ( #42 ) that I'd like to get in place before we merge this.

In the mean time if possible please would you add a changelog fragment to your PR: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to

Given the number of shards required to trigger this I'm not sure we can practically test this properly without delving into the realms of unit tests and I'm not a big fan of unit tests for modules so I'm inclined to wave that requirement.

tremble
tremble approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm going to wait on getting the integration tests in place before merging.

@ansibullbot
Copy link

@sidpatel13 this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed community_review small_patch Hopefully easy to review labels Aug 28, 2020
@sidpatel13 sidpatel13 force-pushed the fix-ansible-kinesis-stream-paginated-shards-bug branch from add42b2 to d1e64de Compare August 31, 2020 21:51
@sidpatel13
Copy link
Contributor Author

@ansibullbot rebased!

@sidpatel13
Copy link
Contributor Author

@tremble @ansibullbot any updates on when this will get merged?

@tremble
Copy link
Contributor

tremble commented Oct 8, 2020

Sorry it took so long, We've got the initial test suite in now and I've triggered a run. I'll manually run through the results (there are some known bugs) and then we can get this merged.

@ansibullbot ansibullbot added community_review plugins plugin (any type) and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_triage labels Nov 16, 2020
@ansibullbot ansibullbot added the small_patch Hopefully easy to review label Nov 16, 2020
@tremble tremble merged commit 601505d into ansible-collections:main Dec 1, 2020
@tremble
Copy link
Contributor

tremble commented Dec 1, 2020

Sorry this took so long to get merged.

@tremble tremble added the pr_day Has been reviewed during a PR review Day. https://github.com/ansible/community/issues/407 label Dec 1, 2020
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@sidpatel13
fix: ansible kinesis stream paginated shards bug (ansible-collections#93
a89557e 
)

* fix: ansible kinesis stream paginated shards bug
* only set shardid params when more shards
* add changelog fragment
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@sidpatel13
fix: ansible kinesis stream paginated shards bug (ansible-collections#93
e331bb8 
)

* fix: ansible kinesis stream paginated shards bug
* only set shardid params when more shards
* add changelog fragment
danielcotton pushed a commit to danielcotton/community.aws that referenced this pull request Nov 23, 2021
@sidpatel13 @danielcotton
fix: ansible kinesis stream paginated shards bug (ansible-collections#93
1d87cdf 
)

* fix: ansible kinesis stream paginated shards bug
* only set shardid params when more shards
* add changelog fragment
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
@tremble
ec2_ami - Ensure suboptions in docs match what's parsed and validated (…
063e62c 
…ansible-collections#93)

* ec2_ami - Ensure suboptions in docs match what's parsed and validated

* Use 2.6 compatible mecahism for removing None entries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tremble tremble tremble approved these changes

Assignees
No one assigned
Labels
affects_2.10 bug This issue/PR relates to a bug community_review module module new_contributor Help guide this first time contributor plugins plugin (any type) pr_day Has been reviewed during a PR review Day. https://github.com/ansible/community/issues/407 small_patch Hopefully easy to review stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ansible kinesis stream paginated shards gets stuck in infinite loop
3 participants
@sidpatel13 @ansibullbot @tremble